Repository navigation
Conversation
A row subscribing a Combine publisher without a .receive(on:) hop gets the CurrentValueSubject bridge's synchronous subscribe-time replay while the LazyVStack is realizing the row — inside an in-flight SwiftUI layout transaction — and its willSet-time emission delivers mid-update. Either path lets the onReceive action write row @State inside the transaction being laid out: the manaflow-ai#2586/manaflow-ai#6556 write-during-layout livelock family. Stable v0.64.17 shipped exactly this shape in TabItemView via tabManager.selectedTabIdPublisher; the guard goes red on this commit by design (two-commit red/green policy) and the fix lands in the next commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TabItemView's .onReceive of tabManager.selectedTabIdPublisher was the one row subscription without a .receive(on:) hop (its two sibling publishers below it both have one). The publisher is a CurrentValueSubject bridge, so it replays the current value synchronously at subscribe time — and a lazy sidebar row subscribes while the LazyVStack realizes it, inside an in-flight SwiftUI layout transaction — and it emits during selectedTabId's willSet, so a selection change made mid-update also delivers mid-update. Either path writes the row's @State observedIsActive inside the transaction being laid out: the manaflow-ai#2586/manaflow-ai#6556 write-during-layout family that livelocked stable v0.64.17 in the wild (62-minute flushTransactions hang, force-quit; see PR body for captures). Delivering on RunLoop.main keeps the write out of the transaction. No visual change: first render reads the live fallback (observedIsActive ?? (tabManager.selectedTabId == tab.id)) and row onAppear seeds the same value, so the deferred replay is deduplicated by updateObservedActiveState's equality guard. ContentView.swift grows by 7 lines (operator + constraint comment); budget updated 16427 -> 16434 to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@gadgetsam is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a ChangesSync onReceive Fix and Lint Guard
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@gadgetsam I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,631 of the 240,000 allowed lines of code this month. Reviews resume on 1 August 2026 (in 28 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
✅ Action performedReview finished.
|
|
To use Codex here, create a Codex account and connect to github. |
1 similar comment
|
To use Codex here, create a Codex account and connect to github. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_ci_sidebar_lazy_layout_guard.py`:
- Around line 27-33: The new onReceive test cases and their docstring entries
reuse case label "(o)", which collides with an existing unrelated case later in
the file. Update the identifiers for the new synchronous/deferred subscription
tests in the test class and matching docstring entries to the next unused
letters so each scenario has a unique cross-reference, keeping the labels in
sync with the existing whole-file row-wrapper scan case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 640d56b0-fa6c-4199-a3cb-513f1a852474
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (3)
Sources/ContentView.swiftscripts/check-sidebar-lazy-layout.pytests/test_ci_sidebar_lazy_layout_guard.py
| (o) An `.onReceive(` in a row whose publisher lacks a `.receive(on:)` hop | ||
| fails — a CurrentValueSubject bridge replays synchronously while the | ||
| LazyVStack realizes the row (and emits during willSet), so the action | ||
| writes row @State inside the in-flight layout transaction. This exact | ||
| shape shipped in stable v0.64.17 via `selectedTabIdPublisher`. | ||
| (p) The same subscription routed through `.receive(on: RunLoop.main)` | ||
| passes, including when `.receive(on:)` spans multiple lines. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate case letter "(o)" reused for two different tests.
The new cases at lines 323-343 (and their docstring entries at lines 27-33) reuse label "(o)", which is already used later in the file (line 392: # (o) Whole-file row-wrapper scan...) for an unrelated pre-existing test. Two different test scenarios now share the same letter, making it harder to cross-reference a failing case back to the docstring.
Consider relettering the new sync/deferred onReceive cases (e.g., to the next unused letters) to avoid collision with the existing wrapper-scan case.
Also applies to: 323-343
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_ci_sidebar_lazy_layout_guard.py` around lines 27 - 33, The new
onReceive test cases and their docstring entries reuse case label "(o)", which
collides with an existing unrelated case later in the file. Update the
identifiers for the new synchronous/deferred subscription tests in the test
class and matching docstring entries to the next unused letters so each scenario
has a unique cross-reference, keeping the labels in sync with the existing
whole-file row-wrapper scan case.
Greptile SummaryThis PR fixes a livelock family (
Confidence Score: 4/5The Swift change is a one-operator addition that aligns the last unguarded subscription with its two already-fixed siblings; the guard script and meta-tests are additive and do not touch any production runtime path. The fix is minimal, well-motivated, and consistent with the existing pattern at the same call site. The two observations (an unverified test assertion about action-closure masking, and a theoretical false-positive from unbalanced parens in malformed Swift) are both in the guard tooling, not in the production Swift path, and neither affects runtime correctness. The guard script ( Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant LVS as LazyVStack (layout tx)
participant TIV as TabItemView
participant CVS as CurrentValueSubject
participant RLM as RunLoop.main
note over LVS,TIV: Row realization (in-flight layout transaction)
LVS->>TIV: realize row (subscribe)
alt BEFORE fix — synchronous delivery
TIV->>CVS: subscribe
CVS-->>TIV: replay current value SYNCHRONOUSLY
TIV->>TIV: "@State write mid-transaction"
note over TIV: write-during-layout livelock #2586
end
alt AFTER fix — delivery hopped to RunLoop.main
TIV->>CVS: subscribe + .receive(on: RunLoop.main)
CVS-->>RLM: enqueue delivery (deferred)
note over LVS: layout transaction completes
RLM-->>TIV: deliver isSelected (next cycle)
TIV->>TIV: "@State write — safe"
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant LVS as LazyVStack (layout tx)
participant TIV as TabItemView
participant CVS as CurrentValueSubject
participant RLM as RunLoop.main
note over LVS,TIV: Row realization (in-flight layout transaction)
LVS->>TIV: realize row (subscribe)
alt BEFORE fix — synchronous delivery
TIV->>CVS: subscribe
CVS-->>TIV: replay current value SYNCHRONOUSLY
TIV->>TIV: "@State write mid-transaction"
note over TIV: write-during-layout livelock #2586
end
alt AFTER fix — delivery hopped to RunLoop.main
TIV->>CVS: subscribe + .receive(on: RunLoop.main)
CVS-->>RLM: enqueue delivery (deferred)
note over LVS: layout transaction completes
RLM-->>TIV: deliver isSelected (next cycle)
TIV->>TIV: "@State write — safe"
end
Reviews (1): Last reviewed commit: "Sidebar: hop selectedTabId row delivery ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # (o) An .onReceive( whose publisher chain has no .receive(on:) hop: | ||
| # the CurrentValueSubject bridge replays synchronously during lazy row | ||
| # realization and emits during willSet, so the action's @State write | ||
| # lands inside the in-flight layout transaction (the #2586/#6556 | ||
| # family). This shape shipped in stable v0.64.17 and livelocked in the | ||
| # wild on 2026-07-02/03. A .receive(on:) inside the ACTION closure | ||
| # (outside the publisher argument) must not mask the violation. | ||
| sync_onreceive_row = row_fixture( | ||
| " HStack { Text(tab.title) }\n" | ||
| " .onReceive(\n" | ||
| " tabManager.selectedTabIdPublisher\n" | ||
| " .map { $0 == tab.id }\n" | ||
| " .removeDuplicates()\n" | ||
| " ) { isSelected in\n" | ||
| " observedIsActive = isSelected\n" | ||
| " }" | ||
| ) | ||
| failures += 0 if expect( | ||
| run_guard(write_fixture(workdir, "SyncOnReceiveRow.swift", sync_onreceive_row)), | ||
| False, "row .onReceive without .receive(on:) fails", | ||
| ) else 1 | ||
|
|
||
| # (p) The same subscription with a .receive(on: RunLoop.main) hop in | ||
| # the publisher chain passes -- also with the hop split across lines, | ||
| # since the real call sites chain one operator per line. |
There was a problem hiding this comment.
The case (o) comment states "A .receive(on:) inside the ACTION closure (outside the publisher argument) must not mask the violation," and the guard's docstring (find_sync_onreceive) makes the same guarantee. However, the sync_onreceive_row fixture has no .receive(on:) anywhere — not in the publisher, not in the action — so the fixture only proves the basic "missing hop fails" path and does not actually exercise the masking scenario. A future regression where the balanced-paren scan accidentally consumed the action closure would silently pass this test. Adding a second fixture that places .receive(on: RunLoop.main) inside the action body (but not the publisher) and asserts expect(..., False, ...) would close this gap.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def find_sync_onreceive(region): | ||
| """Return True if ``region`` (neutralized Swift) contains an | ||
| ``.onReceive(`` whose publisher argument lacks a ``.receive(on:`` hop. | ||
|
|
||
| The publisher expression is the balanced-parenthesis argument list of the | ||
| ``.onReceive(`` call; the action trailing closure sits outside it, so a | ||
| ``.receive(on:)`` inside the action cannot mask a synchronous publisher. | ||
| """ | ||
| for match in ONRECEIVE_CALL.finditer(region): | ||
| i = match.end() - 1 # at the opening '(' of the argument list | ||
| depth = 0 | ||
| start = i | ||
| n = len(region) | ||
| while i < n: | ||
| ch = region[i] | ||
| if ch == "(": | ||
| depth += 1 | ||
| elif ch == ")": | ||
| depth -= 1 | ||
| if depth == 0: | ||
| break | ||
| i += 1 | ||
| publisher_expr = re.sub(r"\s+", "", region[start:i + 1]) | ||
| if ".receive(on:" not in publisher_expr: | ||
| return True | ||
| return False |
There was a problem hiding this comment.
If region contains an .onReceive( whose opening ( is never balanced — for example because a macro or string interpolation produced a lone ( that the neutralizer didn't collapse — the while i < n loop exits with depth > 0 (without hitting the break). At that point i == n, so publisher_expr = re.sub(r"\s+", "", region[start:n+1]) is effectively the rest of the file from the ( onward. That's unlikely to contain .receive(on:, so the function would incorrectly return True and emit a false positive on otherwise clean code. A small guard like if i >= n: continue (skipping unbalanced matches) before the publisher_expr extraction would make the failure mode more predictable, though this scenario would only arise from malformed Swift that the compiler would also reject.
Greptile SummaryThis PR fixes a live hang in stable v0.64.17 by routing
Confidence Score: 4/5The Swift change is a single chained operator on an existing subscription, safe to merge; the guard and tests are sound with two small coverage gaps in the test harness. The production Swift diff is minimal and correct — one tests/test_ci_sidebar_lazy_layout_guard.py — test case (o) comment overstates what the fixture proves, and there is no test for the scan_all_rows branch of the new guard. Important Files Changed
Reviews (2): Last reviewed commit: "Sidebar: hop selectedTabId row delivery ..." | Re-trigger Greptile |
Greptile SummaryAdds
Confidence Score: 4/5Safe to merge; the Swift change is a single chained operator matching the existing pattern at the same call site, and the guard addition has no production runtime impact. The Swift fix is minimal, well-motivated by captured hang samples, and consistent with both sibling publishers. The guard script logic is sound — balanced-paren extraction correctly isolates the publisher argument from the action closure. The only gap is that test case (o)'s stated masking guarantee (.receive(on:) in the action cannot hide a synchronous publisher) is described but not covered by a concrete fixture; the algorithm is correct, but the property is untested. tests/test_ci_sidebar_lazy_layout_guard.py — the action-masking property asserted in the (o) comment should have a companion fixture to remain a live test contract. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant LS as LazyVStack (layout tx)
participant TIV as TabItemView (row)
participant CSP as selectedTabIdPublisher (CurrentValueSubject)
participant RL as RunLoop.main
Note over LS,TIV: Row realization during layout transaction
LS->>TIV: realize row (subscribe)
TIV->>CSP: .onReceive subscribe
alt Before fix (synchronous replay)
CSP-->>TIV: replays current value SYNCHRONOUSLY
TIV-->>TIV: "@State write INSIDE layout transaction"
Note over LS,TIV: write-during-layout livelock (#2586)
end
alt After fix (.receive(on: RunLoop.main))
CSP-->>RL: value deferred to RunLoop.main
Note over LS,TIV: layout transaction completes safely
RL-->>TIV: deliver value on next RunLoop cycle
TIV-->>TIV: "@State write OUTSIDE layout transaction"
end
Note over TIV: onAppear seeds observedIsActive synchronously
Note over TIV: updateObservedActiveState equality guard deduplicates replay
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant LS as LazyVStack (layout tx)
participant TIV as TabItemView (row)
participant CSP as selectedTabIdPublisher (CurrentValueSubject)
participant RL as RunLoop.main
Note over LS,TIV: Row realization during layout transaction
LS->>TIV: realize row (subscribe)
TIV->>CSP: .onReceive subscribe
alt Before fix (synchronous replay)
CSP-->>TIV: replays current value SYNCHRONOUSLY
TIV-->>TIV: "@State write INSIDE layout transaction"
Note over LS,TIV: write-during-layout livelock (#2586)
end
alt After fix (.receive(on: RunLoop.main))
CSP-->>RL: value deferred to RunLoop.main
Note over LS,TIV: layout transaction completes safely
RL-->>TIV: deliver value on next RunLoop cycle
TIV-->>TIV: "@State write OUTSIDE layout transaction"
end
Note over TIV: onAppear seeds observedIsActive synchronously
Note over TIV: updateObservedActiveState equality guard deduplicates replay
Reviews (2): Last reviewed commit: "Sidebar: hop selectedTabId row delivery ..." | Re-trigger Greptile |
| # (o) An .onReceive( whose publisher chain has no .receive(on:) hop: | ||
| # the CurrentValueSubject bridge replays synchronously during lazy row | ||
| # realization and emits during willSet, so the action's @State write | ||
| # lands inside the in-flight layout transaction (the #2586/#6556 | ||
| # family). This shape shipped in stable v0.64.17 and livelocked in the | ||
| # wild on 2026-07-02/03. A .receive(on:) inside the ACTION closure | ||
| # (outside the publisher argument) must not mask the violation. | ||
| sync_onreceive_row = row_fixture( | ||
| " HStack { Text(tab.title) }\n" | ||
| " .onReceive(\n" | ||
| " tabManager.selectedTabIdPublisher\n" | ||
| " .map { $0 == tab.id }\n" | ||
| " .removeDuplicates()\n" | ||
| " ) { isSelected in\n" | ||
| " observedIsActive = isSelected\n" | ||
| " }" | ||
| ) | ||
| failures += 0 if expect( | ||
| run_guard(write_fixture(workdir, "SyncOnReceiveRow.swift", sync_onreceive_row)), | ||
| False, "row .onReceive without .receive(on:) fails", | ||
| ) else 1 |
There was a problem hiding this comment.
The comment asserts that "A .receive(on:) inside the ACTION closure (outside the publisher argument) must not mask the violation," but the fixture has no .receive(on:) in the action at all — it only tests the straightforward "no hop anywhere" path. The balanced-paren extraction is what prevents the bypass, but that specific property is never actually exercised by this fixture. A supplementary sub-test that places .receive(on: RunLoop.main) inside the trailing closure body (e.g., Just(isSelected).receive(on: RunLoop.main).sink { … }) while keeping the publisher argument unhopped would verify the claim and protect it against future refactors of find_sync_onreceive.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| # (o) An .onReceive( whose publisher chain has no .receive(on:) hop: | ||
| # the CurrentValueSubject bridge replays synchronously during lazy row | ||
| # realization and emits during willSet, so the action's @State write | ||
| # lands inside the in-flight layout transaction (the #2586/#6556 | ||
| # family). This shape shipped in stable v0.64.17 and livelocked in the | ||
| # wild on 2026-07-02/03. A .receive(on:) inside the ACTION closure | ||
| # (outside the publisher argument) must not mask the violation. | ||
| sync_onreceive_row = row_fixture( | ||
| " HStack { Text(tab.title) }\n" | ||
| " .onReceive(\n" | ||
| " tabManager.selectedTabIdPublisher\n" | ||
| " .map { $0 == tab.id }\n" | ||
| " .removeDuplicates()\n" | ||
| " ) { isSelected in\n" | ||
| " observedIsActive = isSelected\n" | ||
| " }" | ||
| ) | ||
| failures += 0 if expect( | ||
| run_guard(write_fixture(workdir, "SyncOnReceiveRow.swift", sync_onreceive_row)), | ||
| False, "row .onReceive without .receive(on:) fails", | ||
| ) else 1 | ||
|
|
||
| # (p) The same subscription with a .receive(on: RunLoop.main) hop in | ||
| # the publisher chain passes -- also with the hop split across lines, | ||
| # since the real call sites chain one operator per line. | ||
| deferred_onreceive_row = row_fixture( | ||
| " HStack { Text(tab.title) }\n" | ||
| " .onReceive(\n" | ||
| " tabManager.selectedTabIdPublisher\n" | ||
| " .map { $0 == tab.id }\n" | ||
| " .removeDuplicates()\n" | ||
| " .receive(\n" | ||
| " on: RunLoop.main\n" | ||
| " )\n" | ||
| " ) { isSelected in\n" | ||
| " observedIsActive = isSelected\n" | ||
| " }" | ||
| ) | ||
| failures += 0 if expect( | ||
| run_guard(write_fixture(workdir, "DeferredOnReceiveRow.swift", deferred_onreceive_row)), | ||
| True, "row .onReceive with .receive(on:) passes", | ||
| ) else 1 |
There was a problem hiding this comment.
In the file the execution sequence is (l) → (o) → (p) → (n), which breaks the alphabetical numbering that the rest of the test follows and that the module docstring lists. A reader tracing a failure number will find (n) after (o)/(p) in both the docstring and execution order but only if they know to look past them. Inserting the two new cases after (n) (or renaming them to follow the existing sequence) would preserve the invariant that the letter labels and the code order agree.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if find_sync_onreceive(neutralized): | ||
| violations.append( | ||
| "row-wrapper file contains forbidden synchronous delivery: " | ||
| "{0}".format(ROW_SYNC_ONRECEIVE_MESSAGE) | ||
| ) |
There was a problem hiding this comment.
scan_all_rows branch of find_sync_onreceive has no meta-test coverage
Test cases (o) and (p) exercise find_sync_onreceive only through the type-body extraction path (a TabItemView struct is extracted and its body scanned). The scan_all_rows=True branch here — used for VerticalTabsSidebar+WorkspaceGroups.swift — calls find_sync_onreceive(neutralized) on the whole file, but no meta-test creates a wrapper-file fixture with a bare .onReceive( to verify that branch fires. Currently neither target file has any .onReceive calls, so the gap has no production impact today; if the wrapper file gains a synchronous subscription in future the guard would catch it, but only if this branch hasn't silently broken in the interim.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| # (o) An .onReceive( whose publisher chain has no .receive(on:) hop: | ||
| # the CurrentValueSubject bridge replays synchronously during lazy row | ||
| # realization and emits during willSet, so the action's @State write | ||
| # lands inside the in-flight layout transaction (the #2586/#6556 | ||
| # family). This shape shipped in stable v0.64.17 and livelocked in the | ||
| # wild on 2026-07-02/03. A .receive(on:) inside the ACTION closure | ||
| # (outside the publisher argument) must not mask the violation. | ||
| sync_onreceive_row = row_fixture( | ||
| " HStack { Text(tab.title) }\n" | ||
| " .onReceive(\n" | ||
| " tabManager.selectedTabIdPublisher\n" | ||
| " .map { $0 == tab.id }\n" | ||
| " .removeDuplicates()\n" | ||
| " ) { isSelected in\n" | ||
| " observedIsActive = isSelected\n" | ||
| " }" | ||
| ) | ||
| failures += 0 if expect( | ||
| run_guard(write_fixture(workdir, "SyncOnReceiveRow.swift", sync_onreceive_row)), | ||
| False, "row .onReceive without .receive(on:) fails", | ||
| ) else 1 |
There was a problem hiding this comment.
The inline comment says "A .receive(on:) inside the ACTION closure (outside the publisher argument) must not mask the violation," but the sync_onreceive_row fixture's action body is observedIsActive = isSelected — no .receive(on:) in sight. The algorithm is correct by design (the trailing closure sits outside the balanced parentheses, so publisher_expr never includes it), but the stated property isn't actually exercised. A companion fixture with .receive(on:) only in the action would make this guarantee explicit and prevent a future algorithm refactor from silently regressing it.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Thanks for this! Sidebar rows are snapshot-driven now, so the onReceive livelock is gone landed on main in #8211. You opened this first, so you got there first. Closing since main covers it now. |
2 participants
Summary
Problem. Stable v0.64.17 livelocks in the wild with the #2586 signature. On my machine (0.64.17 (97), macOS 26.5, ~20–40 workspaces driving per-second tmux title updates) macOS wrote three
.hangreports in two days — 173 s, 620 s, and one 62-minute hang force-quit from the Dock. Two livesamplecaptures taken 83 s apart during a fourth hang are statistically identical: 6586/6586 and 3178/3178 main-thread samples inside one never-returningNSHostingView.beginTransaction → GraphHost.flushTransactions, ~90 % underAG::Subgraph::updatedoingLazySubviewPlacements.placeSubviews → LazyStack.place → ForEachList.applyNodesover the workspace-list ForEach, plus ~9 % applyingLazyLayoutCacheItem.AllItemsPhaseMutation— the re-enqueue that keeps the flush from draining. 3–27TerminalController.v2MainSyncsocket threads sit turnstile-blocked behind the busy main thread, so the CLI and Claude hooks hang inread()too. I can attach the .hang files and samples (14–16 MB each) to #2586.This is not a re-fix of #7117 — I'm aware #7117 removed the v0.64.17 row-height probes and hover tracker one day after the release was cut, and #7221 added the scale gate. This PR closes the one residual synchronous-delivery channel that survived, and bans the shape in CI:
Root cause (residual).
TabItemView's.onReceive(tabManager.selectedTabIdPublisher …)is the only row subscription without a.receive(on:)hop (both sibling publishers at the same call site have one). The publisher is aCurrentValueSubjectbridge: it replays synchronously at subscribe time — and a lazy row subscribes while the LazyVStack realizes it, inside an in-flight layout transaction — and it emits duringselectedTabId's willSet, so a selection change made mid-update delivers mid-update. Either path writes the row's@State observedIsActiveinside the transaction being laid out: the same write-during-layout family as #2586/#6556.Fix. Route the chain through
.receive(on: RunLoop.main), matching its two siblings. No visual change: first render reads the live fallback (observedIsActive ?? (tabManager.selectedTabId == tab.id)), rowonAppearseeds the same value, and the deferred replay is deduplicated byupdateObservedActiveState's equality guard.Guard.
scripts/check-sidebar-lazy-layout.pynow fails on any row-region.onReceive(whose publisher argument lacks.receive(on:(balanced-paren extraction, so a hop inside the action closure can't mask a synchronous publisher). Two new meta-test cases (o)/(p) prove catch + pass, including a multi-line.receive(\n on:…).Two-commit red/green (per repo policy): commit 1 adds the guard + meta-tests only —
check-sidebar-lazy-layout.pygoes red onTabItemView(and meta-case (a) with it, by design); commit 2 fixes the call site — everything green.Refs #2586 (evidence lineage: #5323 → #5764 → #5845 → #6033/#6210 → #6556/#7117/#7221).
Testing
Honest scale/build note: I'm an external contributor without Xcode on this machine, so I could not run
reload.shorSidebarLazyLayoutScaleTestslocally — requesting CI as the build/behavior gate. The Swift diff is a single chained operator plus a constraint comment, shape-identical to the sibling subscription at the same call site. Localization audit: no user-facing strings changed.Demo Video
Not applicable — no visual behavior change; this hardens delivery timing for a captured live hang (sample excerpts above, full captures available on request). #7117 precedent: this class is "not cleanly unit-testable without an on-device sample".
Review Trigger
Will post
@codex review / @coderabbitai review / @greptile-apps review / @cubic-dev-ai reviewas a comment after the latest commit.Checklist
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes a sidebar livelock by deferring row selection updates to RunLoop.main and adds a CI guard to block synchronous onReceive delivery in row views. Prevents hangs caused by
selectedTabIdPublisherdelivering during layout.TabItemView, routetabManager.selectedTabIdPublisherthrough.receive(on: RunLoop.main)to avoid synchronousonReceiveupdates during layout. No visual change; first render uses the existing fallback and updates are deduped..onReceive(whose publisher lacks.receive(on:), plus tests (including multiline.receive(on:)) to catch and prevent regressions.Written for commit 2d87b47. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests